Skip to content

Count infinite histogram samples once in edge buckets - #5361

Open
sylvesterkaczmarek wants to merge 3 commits into
NVIDIA:mainfrom
sylvesterkaczmarek:fix/histogram-infinity-counts-20261004
Open

Count infinite histogram samples once in edge buckets#5361
sylvesterkaczmarek wants to merge 3 commits into
NVIDIA:mainfrom
sylvesterkaczmarek:fix/histogram-infinity-counts-20261004

Conversation

@sylvesterkaczmarek

@sylvesterkaczmarek sylvesterkaczmarek commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

Fixes #5360.

Description

Remove the extra degenerate infinity bucket from get_std_histogram_buckets: those samples have already been included in the first or last bucket. Make the positive-infinity check independent of the negative-infinity check so a one-bin histogram can include both endpoints.

The result retains the requested number of bins and counts each infinite sample once. Finite input handling, explicit-range behavior and NaN exclusion remain unchanged. Automatic range inference with infinite inputs is not changed.

Validation

Ten new cases cover negative/positive infinities separately and together, one-bin and all-infinite inputs, finite controls, NaNs, out-of-range finite values, empty inputs, read-only strided views and nonmutation. A dataframe-to-global-histogram check verifies two client datasets produce counts [3, 1, 3], totaling their seven nonmissing samples.

  • New tests against unchanged main: 7 failed, 3 passed.
  • Fixed revision, complete common-statistics test directory: 83 passed, 12 skipped.
  • Scoped equivalents of the style checks in ./runtest.sh -s passed on both changed files: Black, isort and flake8. git diff --check passed.
python -m pytest -o addopts= -q tests/unit_test/app_common/statistics/

Tested on macOS with Python 3.12.11 and NumPy 2.5.3. The full runtest.sh suite and a deployed federated job were not run. No dependency, workflow or configuration changes.

Types of changes

  • Non-breaking correction to histogram counts.
  • New regression tests added.
  • Targeted statistics tests and scoped style checks passed locally.

@sylvesterkaczmarek
Count infinite histogram samples once in edge buckets
f8d1505 
Signed-off-by: Sylvester Kaczmarek <16242628+sylvesterkaczmarek@users.noreply.github.com>
@sylvesterkaczmarek

sylvesterkaczmarek commented Oct 4, 2026

Copy link
Copy Markdown
Contributor Author

@yhwen @IsaacYangSLA The histogram regression tests and common-statistics suite pass locally (83 passed, 12 skipped), along with scoped style checks. Could you review this and trigger /build when appropriate?

@greptile-apps

greptile-apps Bot commented Oct 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[Medium impact] The PR appears safe to merge; no new actionable issues were found.

Summary

The PR counts each infinite sample once in an outer histogram bin and keeps the requested number of bins.

  • Standard histograms keep infinite samples in the requested edge bins.
  • Global histograms combine client bins with infinite outer edges.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Client samples] --> B[Build requested histogram bins]
    B --> C[Add negative infinity to first bin]
    C --> D[Add positive infinity to last bin]
    D --> E[Combine client histograms]
    E --> F{Only infinite outer edges differ?}
    F -->|Yes| G[Keep infinite edges and add counts]
    F -->|No| H[Use existing exact-range merge]
    G --> I[Global histogram]
    H --> I
Loading

Reviews (3) · Last reviewed commit: "Merge branch 'main' into fix/histogram-i..." · Reviewed by Greptile

greptile-apps[bot]
greptile-apps Bot reviewed Oct 4, 2026
View reviewed changes
Comment thread nvflare/app_common/statistics/numpy_utils.py
This was referenced Oct 7, 2026
Closed
Open
IsaacYangSLA
IsaacYangSLA requested changes Oct 8, 2026
View reviewed changes

@IsaacYangSLA IsaacYangSLA left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing mixed-client histogram finding remains reproducible at this head: #5361 (comment).

With one histogram bin over (-1, 1), clients containing [-inf, 0] and [-inf, 0, inf] aggregate to 2 samples, or 3 with the clients reversed, instead of 5. Endpoint bounds depend on each client's infinity signs, while accumulate_hists matches bins by their exact ranges and retains the first client's ranges. The newly independent positive-endpoint condition makes these one-bin keys differ, so a client's entire contribution disappears. Please keep bin ranges compatible across clients or reconcile their ranges during aggregation, and cover differing infinity patterns in an integration test.

The 24 changed histogram tests pass, but their aggregation case uses the same infinity pattern at both clients and misses this scenario. This review confirms the existing issue without adding a duplicate inline comment.

@sylvesterkaczmarek
Preserve mixed-client infinity histogram counts
f7d7c3d 
Signed-off-by: Sylvester Kaczmarek <16242628+sylvesterkaczmarek@users.noreply.github.com>
@sylvesterkaczmarek

sylvesterkaczmarek commented Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

@IsaacYangSLA Thanks for reproducing the mixed-client failure. Addressed in f7d7c3d: accumulate_hists now reconciles standard histograms whose grids match except for -inf/+inf outer-edge extensions, preserving all sample counts across clients. Different interior grids and finite-to-finite endpoint differences are not silently aligned. Added 12 parameterized dataframe/global-aggregation regressions covering your exact one-bin example, mixed finite/infinite clients, three-bin histograms, and both client orders. On the previous head 11 of these fail; on the updated head all 12 pass. Full focused statistics suite: 95 passed, 12 skipped. Black, isort, flake8 and diff checks pass. Could you re-review the updated head and run /build when appropriate?

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@greptile-apps greptile-apps[bot] greptile-apps[bot] left review comments
@IsaacYangSLA IsaacYangSLA Awaiting requested review from IsaacYangSLA

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Federated statistics histograms count infinite samples twice

3 participants

@sylvesterkaczmarek @IsaacYangSLA @YuanTingHsieh